Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SQL: Fix display size for DATE/DATETIME #40669

Merged
merged 4 commits into from
Apr 3, 2019
Merged

SQL: Fix display size for DATE/DATETIME #40669

merged 4 commits into from
Apr 3, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 30, 2019

A full format for a DATETIME would be:
2019-03-30T10:20:30.123456789+10:00 (example when asking for
current_timestamp(9)) which is 35 chars long.

For DATE a full format would be: 2019-03-30T00:00:00.000+10:00
which is 29 chars long.

@matriv
SQL: Fix displa size for DATE/DATETIME
c4b10b6 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123456789+10:00` (example when asking for
`current_timestamp(9)`) which is 35 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is 29 chars long.
@matriv matriv requested review from costin and astefan March 30, 2019 15:56
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv matriv changed the title SQL: Fix displa size for DATE/DATETIME SQL: Fix display size for DATE/DATETIME Mar 30, 2019
astefan
astefan approved these changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a comment about the protocol tests.

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/SqlProtocolTestCase.java Outdated
@@ -67,11 +67,16 @@ public void testTextualType() throws IOException {

public void testDateTimes() throws IOException {
assertQuery("SELECT CAST('2019-01-14T12:29:25.000Z' AS DATETIME)", "CAST('2019-01-14T12:29:25.000Z' AS DATETIME)",
"datetime", "2019-01-14T12:29:25.000Z", 24);
"datetime", "2019-01-14T12:29:25.000Z", 35);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the precision changed, please change the tests' values or add new tests (preferred) to reflect the increased precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this revealed another bug: #40692
so currently I cannot have easily a test with custom timezone.

@matriv
Remove 6 digits from precision.
86b7423 
Currently we only display down to millis.
@matriv matriv requested a review from astefan April 1, 2019 16:14
@matriv
Copy link
Contributor Author

matriv commented Apr 1, 2019

/cc @bpintea We keep the precision of second fractional digits to 3 as currently we never return more than that. Added a comment to docs regarding CURRENT_TIMESTAMP(x) where x > 3 (=> no effect, max = 3). Increased the precision to include timezone info like +05:00 or -10:00.

astefan
astefan reviewed Apr 1, 2019
View reviewed changes
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java Outdated
@@ -44,11 +44,11 @@
OBJECT( "object", JDBCType.STRUCT, -1, 0, 0, false, false, false),
NESTED( "nested", JDBCType.STRUCT, -1, 0, 0, false, false, false),
BINARY( "binary", JDBCType.VARBINARY, -1, Integer.MAX_VALUE, Integer.MAX_VALUE, false, false, false),
DATE( JDBCType.DATE, Long.BYTES, 24, 24, false, false, true),
DATE( JDBCType.DATE, Long.BYTES, 3, 29, false, false, true),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The precision here (3) and the comment below (about display size and precision) are incompatible I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3 shows how many second fractional digits are used. currently millis -> 3. This was fixed here: 1557d77#diff-a3aa9a4b764c281befa4ee231082c298R51

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks. Then the code comment is inaccurate or not necessary?

  1. The precision is not 23 + 1 anymore
  2. the timezone is not Z only anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. Thx will fix it.

astefan
astefan approved these changes Apr 1, 2019
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I left a comment.

@matriv matriv merged commit 6be8396 into elastic:master Apr 3, 2019
@matriv matriv deleted the mt/fix-datetime-precision branch April 3, 2019 11:28
matriv added a commit that referenced this pull request Apr 3, 2019
@matriv
SQL: Fix display size for DATE/DATETIME (#40669)
952c4d9 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123+10:00` which is 29 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is also 29 chars long.


(cherry picked from commit 6be8396)
matriv added a commit that referenced this pull request Apr 3, 2019
@matriv
SQL: Fix display size for DATE/DATETIME (#40669)
c119b8f 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123+10:00` which is 29 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is also 29 chars long.

(cherry picked from commit 6be8396)
matriv added a commit that referenced this pull request Apr 3, 2019
@matriv
SQL: Fix display size for DATE/DATETIME (#40669)
aa076ab 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123+10:00` which is 29 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is also 29 chars long.

(cherry picked from commit 6be8396)
matriv added a commit that referenced this pull request Apr 3, 2019
@matriv
SQL: Fix display size for DATE/DATETIME (#40669)
67e5af6 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123+10:00` which is 29 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is also 29 chars long.

(cherry picked from commit 6be8396)
@matriv
Copy link
Contributor Author

matriv commented Apr 3, 2019

Backported to 7.x with 952c4d9
to 7.0 with c119b8f
to 6.7 with aa076ab
to 6.6 with 67e5af6

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into build-flavor-type-…
34daff9 
…leniency

* elastic/master:
  SQL: Fix deserialisation issue of TimeProcessor (elastic#40776)
  Improve GCS docs for using keystore (elastic#40605)
  Add Restore Operation to SnapshotResiliencyTests (elastic#40634)
  Small refactorings to analysis components (elastic#40745)
  SQL: Fix display size for DATE/DATETIME (elastic#40669)
  add HLRC protocol tests for transform state and stats (elastic#40766)
  Inline TransportReplAction#registerRequestHandlers (elastic#40762)
  remove experimental label from search_as_you_type documentation (elastic#40744)
  Remove some abstractions from `TransportReplicationAction` (elastic#40706)
  Upgrade to latest build scan plugin (elastic#40702)
  Use default memory lock setting in testing (elastic#40730)
  Add Bulk Delete Api to BlobStore (elastic#40322)
  Remove yaml skips older than 7.0 (elastic#40183)
  Docs: Move id in the java-api (elastic#40748)
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@matriv
SQL: Fix display size for DATE/DATETIME (elastic#40669)
2f0e850 
A full format for a DATETIME would be:
`2019-03-30T10:20:30.123+10:00` which is 29 chars long.

For DATE a full format would be: `2019-03-30T00:00:00.000+10:00`
which is also 29 chars long.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astefan astefan astefan approved these changes

@costin costin Awaiting requested review from costin

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >bug v6.6.3 v6.7.2 v7.0.0-rc2 v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@matriv @elasticmachine @astefan @jakelandis